Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor:notice 날짜 포맷팅 수정 #159

Merged
merged 7 commits into from
Nov 14, 2024
Merged

refactor:notice 날짜 포맷팅 수정 #159

merged 7 commits into from
Nov 14, 2024

Conversation

s-hwan
Copy link
Contributor

@s-hwan s-hwan commented Nov 9, 2024

📌 Related Issue

#157

🚀 Description

  • 공지 날짜 포맷팅 수정했습니다

📢 Review Point

  • 공지 날짜 포맷팅 수정했는데 DateFormatUtil에 공지를 위한 static 함수를 만들어서 했는데.. 잘 한게 맞을지 모르겠습니다

📚Etc (선택)

  • 원래 DateFormatUtil의 함수 포맷만 변경하려 했는데 solution에서 쓰이고 있길래 공지로 따로 만들어서 했습니다 어렵네요..

  • [Q] 이 흐름이 궁금합니다 Notice를 만들면 DB에

	noticeRepository.save(Notice.builder()
			.author(user)
			.studyGroup(studyGroup)
			.title(request.title())
			.content(request.content())
			.createdAt(LocalDateTime.now())
			.build());

이렇게 저장을 해놓는거잖아요? 저장 후에 클라에서 요청을 받으면
Response를

		return GetNoticeResponse.builder()
			.author(notice.getAuthor().getNickname())
			.noticeId(notice.getId())
			.noticeTitle(notice.getTitle())
			.noticeContent(notice.getContent())
			.createAt(DateFormatUtil.formatDateTimeForNotice(notice.getCreatedAt()))
			.build();

이렇게 보내 주는 흐름이 맞는지.. 궁금합니다

@s-hwan s-hwan added the refactoring 리팩토링 label Nov 9, 2024
@s-hwan s-hwan requested review from rladmstn and sh0723 November 9, 2024 11:46
@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 9, 2024

아 왜 마크다운 문법에 맞게 썼는데 안됨 ㅜㅜ

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 9, 2024

아 왜 마크다운 문법에 맞게 썼는데 안됨 ㅜㅜ

성공!

@s-hwan s-hwan self-assigned this Nov 9, 2024
@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 9, 2024

하...

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 9, 2024

로컬에선 돌아가는데 왜 안된다는거니

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 9, 2024

로컬에선 돌아가는데 왜 안된다는거니

성공~!

@sh0723
Copy link
Contributor

sh0723 commented Nov 10, 2024

ㅋㅋㅋㅋㅋ고생하셨네요.. 근데 하나 궁금한게 포멧을 db에 저장할 때부터 바꿔서 저장하면 되지 않을까요?
서비스 중에 전에 쓰던 포멧을 확인하는 서비스가 없다면 그렇게 하는게 개인적으로 더 좋아보이긴해요!

Copy link
Contributor

@rladmstn rladmstn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트 확인 부탁합니다-!

Comment on lines 19 to 20
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yy/MM/dd HH:mm");
return dateTime.format(formatter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 이렇게 포맷하면 뒤에 AM,PM도 붙나요??

image

뷰에 보심 AM, PM도 있습니다..!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 이렇게 포맷하면 뒤에 AM,PM도 붙나요??

image 뷰에 보심 AM, PM도 있습니다..!

요렇게 하면 'AM/PM' 이 안 붙는답니다..!
HH:mm a 라고 해야 AM PM이 붙는다고 합니다 감사합니다~

@rladmstn
Copy link
Contributor

rladmstn commented Nov 12, 2024

아 그리고 코멘트 내용 의견 말씀드리자면!
저는 써두신대로 DB에 데이터 저장할 땐 Date 타입으로 저장하고 DTO에서 포맷팅 적용하는게 적절하다고 생각합니다.
아무래도 우리 서비스는 확장성도 중요시하게 여겨야하고, 추후에 Date 정보를 사용해서 공지글을 정렬한다거나.. 날짜 정보를 활용한 기능이 추가될 수 있음을 고려해서 저는 보통 DB 변경보단 DTO를 변경하는 것을 우선으로 생각하는 편입니다.

image

그리고 한글..로 뜨네요 ㅎㅎ 프론트는 AM/PM으로 원하는 것 같아서..! 확인하고 다시 한 번 수정 부탁합니다!

테스트에서 DateFormatUtil 메소드의 결과랑 같은지를 검증하고 있어요!
결국 DateFormatUtil 메소드가 원하는 형태로 출력되는지는 검증이 되지 않고 있습니다.
아마 이런 것 때문에 테스트 코드에서 발견을 못하셨을 것이라고 생각이 드네요..!

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 12, 2024

아 그리고 코멘트 내용 의견 말씀드리자면! 저는 써두신대로 DB에 데이터 저장할 땐 Date 타입으로 저장하고 DTO에서 포맷팅 적용하는게 적절하다고 생각합니다. 아무래도 우리 서비스는 확장성도 중요시하게 여겨야하고, 추후에 Date 정보를 사용해서 공지글을 정렬한다거나.. 날짜 정보를 활용한 기능이 추가될 수 있음을 고려해서 저는 보통 DB 변경보단 DTO를 변경하는 것을 우선으로 생각하는 편입니다.

image 그리고 한글..로 뜨네요 ㅎㅎ 프론트는 `AM/PM`으로 원하는 것 같아서..! 확인하고 다시 한 번 수정 부탁합니다!

테스트에서 DateFormatUtil 메소드의 결과랑 같은지를 검증하고 있어요! 결국 DateFormatUtil 메소드가 원하는 형태로 출력되는지는 검증이 되지 않고 있습니다. 아마 이런 것 때문에 테스트 코드에서 발견을 못하셨을 것이라고 생각이 드네요..!

똑바로 하겠습니다 ...

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 12, 2024

테스트에서 DateFormatUtil 메소드의 결과랑 같은지를 검증하고 있어요! 결국 DateFormatUtil 메소드가 원하는 형태로 출력되는지는 검증이 되지 않고 있습니다. 아마 이런 것 때문에 테스트 코드에서 발견을 못하셨을 것이라고 생각이 드네요..!

image
수정은 성공했습니다..!
근데 말씀하신

테스트에서 DateFormatUtil 메소드의 결과랑 같은지를 검증하고 있어요!
결국 DateFormatUtil 메소드가 원하는 형태로 출력되는지는 검증이 되지 않고 있습니다.
아마 이런 것 때문에 테스트 코드에서 발견을 못하셨을 것이라고 생각이 드네요..!

이 부분이 이해가 잘 안됩니다 test코드 찾아 봤는데 검증이 되지 않고 있다고 말씀하신 부분이 아랫 부분이 맞나요?

assertThat(response.createAt()).isEqualTo(DateFormatUtil.formatDateTimeForNotice(notice.getCreatedAt()));

맞다면 DB에는 LocalDateTime으로 저장하지만 검증은 포매팅한 결과로 검증해서 잘못됐다고 말씀하신건지 이해가 잘 안됩니다..ㅜㅜㅜ

@rladmstn
Copy link
Contributor

assertThat(response.createAt()).isEqualTo(DateFormatUtil.formatDateTimeForNotice(notice.getCreatedAt()));

맞다면 DB에는 LocalDateTime으로 저장하지만 검증은 포매팅한 결과로 검증해서 잘못됐다고 말씀하신건지 이해가 잘 안됩니다..ㅜㅜㅜ

일단 저 코드 부분 말한 것은 맞습니다!

한글로 출력되는 오류가 테스트에서 검증되지 못했던 이유는 DateFormatUtil.formatDateTimeForNotice() 메서드에 대한 테스트가 아니기 때문입니다.

지금처럼 테스트를 진행하면 공지의 생성 날짜 형태가 저 포맷팅 메소드를 거치는지는 테스트가 되지만, 그 포맷팅 메소드에서 우리가 원하는 형태로 변환해주는지는 모르기 때문이죠!
만약 원했던 날짜 포맷팅이 나오는지를 테스트하고 싶다면, DateFormatUtil.formatDateTimeForNotice()에 대한 테스트를 파서 검증을 해야 했다는 말이었습니다!

아직 이해 안되는 부분 있다면 말씀해주세용

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 13, 2024

assertThat(response.createAt()).isEqualTo(DateFormatUtil.formatDateTimeForNotice(notice.getCreatedAt()));

맞다면 DB에는 LocalDateTime으로 저장하지만 검증은 포매팅한 결과로 검증해서 잘못됐다고 말씀하신건지 이해가 잘 안됩니다..ㅜㅜㅜ

일단 저 코드 부분 말한 것은 맞습니다!

한글로 출력되는 오류가 테스트에서 검증되지 못했던 이유는 DateFormatUtil.formatDateTimeForNotice() 메서드에 대한 테스트가 아니기 때문입니다.

지금처럼 테스트를 진행하면 공지의 생성 날짜 형태가 저 포맷팅 메소드를 거치는지는 테스트가 되지만, 그 포맷팅 메소드에서 우리가 원하는 형태로 변환해주는지는 모르기 때문이죠! 만약 원했던 날짜 포맷팅이 나오는지를 테스트하고 싶다면, DateFormatUtil.formatDateTimeForNotice()에 대한 테스트를 파서 검증을 해야 했다는 말이었습니다!

아직 이해 안되는 부분 있다면 말씀해주세용

이해됐습니다!! 감사합니다 ㅎㅎㅎ

@s-hwan
Copy link
Contributor Author

s-hwan commented Nov 13, 2024

이 pr 머지 해도 되겠습니까?!

@rladmstn
Copy link
Contributor

넴 제가 머지하겠습니다~

@rladmstn rladmstn merged commit 568fb40 into develop Nov 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants